Skip to content

Add an option refusing the implicit flow for confidential clients - #362

Merged
cicnavi merged 4 commits into
simplesamlphp:masterfrom
ioigoume:fix/disallow-confidential-client-implicit-flow
Nov 4, 2026
Merged

cicnavi merged 4 commits into
simplesamlphp:masterfrom
ioigoume:fix/disallow-confidential-client-implicit-flow

Conversation

@ioigoume

@ioigoume ioigoume commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

On v6, this flag is the only way to stop confidential clients from using the implicit flow, because v6 has no per-client response_types or grant_types. v6 also has no Dynamic Client Registration, so the flag covers admin-created and federated clients. On v7, the flag adds to the existing controls (per-client response_types/grant_types and server-wide enabled_grant_types) rather than replacing them. Its main use there is as one server-wide switch for clients whose response_types are still empty: clients migrated from v6, or clients that registered themselves through DCR (off by default). A possible v7 follow-up: when the flag is on, DCR could reject confidential clients that ask for implicit response types, returning invalid_client_metadata, so registration and runtime behave the same.

@ioigoume
ioigoume marked this pull request as draft October 21, 2026 07:24
@ioigoume ioigoume changed the title Fix/disallow confidential client implicit flow Fix/disallow confidential client implicit flow - Global switch Oct 21, 2026
Give OidcServerException::unauthorizedClient() the message, code and
parameter order it has on wip-version-7, which also follow its sibling
methods here.

Say in the comments what the specifications say. RFC 9700 section 2.1.2 is
about access tokens in the authorization response, so it covers 'id_token
token' but not 'id_token' alone, and OpenID Connect Core section 15.4 only
says the authorization code flow may be appropriate for confidential
clients. The option goes beyond both, and enabling it stops any confidential
client which uses the implicit flow today.

Replace the integration test, which CI could not run without a signing key,
with unit tests pinning the whole error response, public clients, the
default, and the option reaching the grant through the factory. Its cases
passed once against this code, with a temporary key. tests/bootstrap.php
goes back to what it was, as its additions served one local layout.

Assisted-by: Claude Code (claude-opus-5-5, rework & unit tests)
@cicnavi cicnavi changed the title Fix/disallow confidential client implicit flow - Global switch Add an option refusing the implicit flow for confidential clients Nov 4, 2026
@cicnavi
cicnavi changed the base branch from version-6 to master November 4, 2026 11:26
@codecov

codecov Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.64%. Comparing base (869eea6) to head (5467925).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/Server/Exceptions/OidcServerException.php 0.00% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #362      +/-   ##
============================================
+ Coverage     53.99%   54.64%   +0.65%     
- Complexity     1512     1516       +4     
============================================
  Files           141      141              
  Lines          6078     6097      +19     
============================================
+ Hits           3282     3332      +50     
+ Misses         2796     2765      -31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cicnavi
cicnavi marked this pull request as ready for review November 4, 2026 11:46
@cicnavi
cicnavi merged commit 93ece63 into simplesamlphp:master Nov 4, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants